-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added new endpoint to paginate a Workflow Run task_logs #177
Conversation
When setting
|
@wleepang that was something I thought of. I was not quite sure how well the polymorphism would sit with client frameworks. I would probably add a key to the run log instead then, maybe |
@patmagee - that's a valid point. Having the additional |
@wleepang I added a new key to the request. I am also specifically trying to preserve current behaviour so as not to trigger a version bump |
This is nice, but I have two questions:
~p |
@pgrosu you raise a very valid point. Some form of filtering would be desirable, however I wanted to mimic the current pagination API as much as possible as well (which does not support arbitrary slices and filters). Maybe there is a different way to communicate failures. If an individual task was addressable (currently it is not, and tasks do not have an id), then we could provide a |
In theory tasks could be auto-identifiable, since you can create a hash of the static elements What if workflow of 10,000+ could in theory be considered tiny -- sounds shocking, right? Usually workflows components could run faster over time as more data is added if things are properly cached at multiple levels of Ask yourself what types of experiments you could run if workflows were almost instantaneous? Now given that -- and ever larger workflows -- how could human-intervention be minimized and ensure that on average they complete on their own? Ideally workflows would pause-and-continue/restart until the correct Details are important, but if we limit ourselves then how ubiquitously will our implementation be adopted? If our worry now about not triggering a version bump, maybe parts of our design might not be proper for the kind of science we might need to be doing, right? Hope it helps, |
I think that setting a task caching and reuse policy is something that can be made in the RunWorkflow request and is probably best suited to a separate discussion. The point of quickly identifying a failed task in a list of 10,000+ is valid and lends me to think about the API pattern of
We have this already with:
Perhaps we should consider:
This provides a relatively small response for |
@wleepang that approach feels much more natural and aligns well with the current behaviour of WES. The only thing I am hesitant about is the necessary introduction of two new properties: Status is something that I would like to have as well. seeing the return code is helpful, but knowing if the wes engine treats that return code as a failure or not is even more helpful IMO. I know in the |
@wleepang I am wondering if we can take a two step approach here to not introduce any sort of hard breaking changes and get this PR merged asap
|
@patmagee I think that is a reasonable plan of action |
@pgrosu what do you think about that approach? |
@patmagee That sounds reasonable to me as well. I don't think many folks will create heavy dependencies on |
@pgrosu @wleepang I have reflected the changes in the |
c65adf6
to
fab514f
Compare
There is only a next-page token field. Generally, when paging, and in particular with 10000+ tasks, you may want to avoid to having to iterate through all pages. Did you consider adding a field for the total task count and a way to address each page, e.g. with a second parameter e.g. Furthermore, I am just wondering how useful it is to have too much task-level information in the WES API. All the information that you list for the tasks (name, command, etc.) could equally be provided by the TES-API. Wouldn't it be more appropriate to reuse the TES-API for TES-specific information -- even in the WES-API? Just a thought. A WES implementation that does not use a TES in the background could still provide this information via a TES-API conform endpoint, or not? |
I missed a lot of this conversation, but I strongly support @vinjana's suggestion of reusing (parts of) the TES API instead of introducing a new |
I like the idea of combining the APIs to provide a richer set of information to the end user, however I feel like it introduces a large amount of complexity to both the end user and the implementor. At the moment as a user for WES, the only tasks I care about for a given workflow are those run actually in a workflow. They are conveniently located in the As an implementor, (especially an implementor that layers on top of other engines), it is simply not possible to have the task level metadata required to build out a TES response for ALL tasks across all workflows at once. In the engine world most data is subsetted at the level of the workflow so you would at least need to slice the task level metadata here. I think there are potentially two different approaches we can take to align with TRS a bit better.
|
Right, I can also not see now how one could ensure that only the tasks of the specific workflow run are being displayed, because this would have to be embedded in the URL pointing to the TES-like route/endpoint, and that API probably does not provide a canned way of doing this (I admit, I am not familiar with the TES-API 😅). Your second suggestion, to put an route and response into the WES specification that is just like some fixed TES versions response makes sense to me. Giving it a second thought, I would find it important though, to allow for page selection. Looking at the TES specification this does not seem to be possible or planned in the moment. Sorry for not checking that before! So maybe I can tone down my suggestion: It would be nice to find a harmonized approach to pagination in WES, TES, and possibly other GA4GH standards that also allows for page selection. I am not sure how realistic this is. |
@vinjana YES I 100% agree
This is what I would really love. At the moment this is all over the place and the specs really feel quite separate. Even the payloads that they return. For example, TRS returns an But this ticket: #183 also talks about pagination and pagination metadata. So I would really love to discuss that |
Re-reading this thread, I thought about a potential alternative to the two approaches for better alignment with TES outlined by @patmagee here. Perhaps we could define a TES URI (similar to TRS URI and DRS URI) and include an optional property I also would like to track back on my initial opinion that I would like to see reusing TES schemas here. I simply forgot/misunderstood that we already have the |
fab514f
to
fc1c0ca
Compare
fc1c0ca
to
4f03dc2
Compare
@uniqueg would you be happy with the current approach as-is. Your proposal could easily be added at a later time in a non breaking, especially since there is no such thing as a TES_URI At the moment |
Yeah sure, absolutely :) |
This field is deprecated and the `task_logs_url` should be used to retrieve a paginated list | ||
of steps from the workflow run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should be specific as to when the field will be removed, e.g., for the next major version release 2.0.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a minor comment on the depreciation warning, but not important.
There does not appear to be any one opposed to merging this into the spec as is, so I am considering this change accepted. Final acceptance of this feature will be done during review of the next release |
An increasingly common usability problem encountered with the WES API is that very large (wide) workflows can become next to impossible to handle if they have a large number of tasks. The current behaviour requires all of the workflow tasks to be returned as part of the body of a
RunLog
request. Embedding 10,000+ tasks in the responsejson
can lead to substantial performance degredation on the side of the client as well as the server.This issue is not strictly present when the number of tasks is beyond a certain threshold. Depending on the WES implementation, retrieving a complete list of all the completed and ongoing tasks could be exceptionally expensive. providing an alternative strategy to page through tasks helps mitigate this problem, allowing these operations to be split up into many smaller request.
This PR Addresses this issue by introducing two new features:
/runs/{run_id}/tasks
to returnTaskListResponse
/runs/{run_id}
Retrieve Task Logs
For the workflow run
10002
, we can retrieve the paginated list of tasks with apage_size
of 2. if the page_size is not defined, then the response should use a default value.Now using the
next_page_token
returned by the previous request, we can fetch the next page of at most 2 tasks from the WES server for this run.Exclude Task Logs from RunLog
An important feature for helping with scalability is the ability to exclude
task_logs
from theRunLog
response body. Adding the separate endpoint only makes sense if this behaviour is toggleable by the end user.The new changes specifies that omitting the
exclude_task_logs
query param is equivalent to explicitly passingexclude_task_logs=false
. In either of these cases, then thetask_logs
MUST be included as per the current behaviour of WES. That is, All logs should be included. This avoids breaking the API and requiring a major version bump.When a user passes in
exclude_task_logs=true
, thetask_logs
property in the response should either be omitted entirely, or set tonull